BLD: use unsigned instead of signed for lengths, avoid build warnings #26759

jbrockmendel · 2019-06-10T03:30:26Z

I'm much more confident about the other two BLD PRs (#26757, #26758) than this, largely because:

in a couple of comments here I wrote that a certain term should always be non-negative. I haven't actually confirmed that this is always the case
This changed things from int64_t to uint64_t, but could also reasonably have changed them to size_t. I don't have a compelling reason for the correctness of the uint64_t choice.

codecov · 2019-06-10T04:10:49Z

Codecov Report

Merging #26759 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #26759      +/-   ##
==========================================
- Coverage   91.71%    91.7%   -0.01%     
==========================================
  Files         178      178              
  Lines       50740    50740              
==========================================
- Hits        46538    46533       -5     
- Misses       4202     4207       +5

Flag	Coverage Δ
#multiple	`90.3% <ø> (ø)`	⬆️
#single	`41.21% <ø> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️
pandas/core/frame.py	`96.88% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.84% <0%> (-0.11%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f3e8e8...ac9536d. Read the comment docs.

codecov · 2019-06-10T04:10:49Z

Codecov Report

Merging #26759 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #26759      +/-   ##
==========================================
- Coverage   91.97%   91.97%   -0.01%     
==========================================
  Files         180      180              
  Lines       50756    50756              
==========================================
- Hits        46685    46682       -3     
- Misses       4071     4074       +3

Flag	Coverage Δ
#multiple	`90.56% <ø> (ø)`	⬆️
#single	`41.84% <ø> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`88.88% <0%> (-11.12%)`	⬇️
pandas/core/frame.py	`96.89% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`90.94% <0%> (+0.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9b081d...fa66545. Read the comment docs.

WillAyd

On board with the concept just question around casts (applicable in a few places)

WillAyd · 2019-06-10T12:38:45Z

pandas/_libs/src/parser/tokenizer.c

@@ -471,7 +472,7 @@ static int end_line(parser_t *self) {
        return 0;
    }

-    if (!(self->lines <= (int64_t) self->header_end + 1) &&
+    if (!(self->lines <= (uint64_t) self->header_end + 1) &&


Is this cast necessary?

Good catch, I think it isn't.

WillAyd · 2019-06-10T12:44:19Z

pandas/_libs/src/parser/tokenizer.c

@@ -248,7 +248,8 @@ void parser_del(parser_t *self) {
 }

 static int make_stream_space(parser_t *self, size_t nbytes) {
-    int64_t i, cap, length;
+    uint64_t i, cap;
+    int64_t length;


Does leaving this as is not introduce any new build warnings? Looks like assignments and comparisons are done with unsigned types within this function

…kenizer

WillAyd · 2019-06-10T20:05:06Z

pandas/_libs/src/parser/tokenizer.c

@@ -651,7 +652,7 @@ static int parser_buffer_bytes(parser_t *self, size_t nbytes) {
    stream = self->stream + self->stream_len;                        \
    slen = self->stream_len;                                         \
    self->state = STATE;                                             \
-    if (line_limit > 0 && self->lines == start_lines + (int64_t)line_limit) {  \
+    if (line_limit > 0 && self->lines == start_lines + (uint64_t)line_limit) { \


Is there a reason to even have these casts? Fraught with peril to begin with using macros I think this just adds nuances / developer burden

Updated to remove these casts.

Should we consider getting rid of the macros? That's outside my region of competence.

Yea might be worth doing so in a separate PR. Here's the cpp take on macros:

https://isocpp.org/wiki/faq/inline-functions#inline-vs-macros

I could see this one being particular troublesome with our codebase:

https://isocpp.org/wiki/faq/misc-technical-issues#macros-with-if

I think the main downside to using inline vs macros would be support for pre-C99 compilers, but I don't think that's a problem given age and the fact that we have inline functions elsewhere in the code base

…kenizer

WillAyd · 2019-06-12T13:24:56Z

OK by me. Does this offer any performance improvements? Wondering if Cython optimizes for not doing a wraparound check like in this SO question:

https://stackoverflow.com/a/37933120/621736

jreback · 2019-06-12T14:38:25Z

pandas/_libs/src/parser/tokenizer.c

@@ -248,7 +248,8 @@ void parser_del(parser_t *self) {
 }

 static int make_stream_space(parser_t *self, size_t nbytes) {
-    int64_t i, cap, length;
+    uint64_t i, cap;
+    int64_t length;


why is length signed?

Because there weren't any warnings telling us not to. Probably makes more sense unsigned liked the others.

can you change this, otherwise lgtm.

Changing this requires changing a few other places to match. I think its benign, but not obvious. Go for it?

yep prob should

jreback · 2019-06-21T02:35:03Z

small comment, merge master and ping on green.

…kenizer

jbrockmendel · 2019-06-22T07:03:15Z

Ping

WillAyd · 2019-06-22T17:19:14Z

Thanks @jbrockmendel !

BLD: use unsigned instead of signed for lengths, avoid build warnings

ac9536d

WillAyd reviewed Jun 10, 2019

View reviewed changes

WillAyd added the Clean label Jun 10, 2019

WillAyd reviewed Jun 10, 2019

View reviewed changes

jbrockmendel added 2 commits June 10, 2019 06:57

Merge branch 'master' of https://github.com/pandas-dev/pandas into to…

6f90c8e

…kenizer

edit for comment and lint

d61082b

WillAyd reviewed Jun 10, 2019

View reviewed changes

jbrockmendel added 2 commits June 11, 2019 06:53

Merge branch 'master' of https://github.com/pandas-dev/pandas into to…

37b6002

…kenizer

remove casts

f29bfdc

WillAyd added this to the 0.25.0 milestone Jun 12, 2019

jreback requested changes Jun 12, 2019

View reviewed changes

jbrockmendel added 2 commits June 20, 2019 19:49

Merge branch 'master' of https://github.com/pandas-dev/pandas into to…

15c1c5d

…kenizer

update to unsigned types

fa66545

WillAyd merged commit 2b9b58d into pandas-dev:master Jun 22, 2019

jbrockmendel deleted the tokenizer branch June 22, 2019 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLD: use unsigned instead of signed for lengths, avoid build warnings #26759

BLD: use unsigned instead of signed for lengths, avoid build warnings #26759

jbrockmendel commented Jun 10, 2019

codecov bot commented Jun 10, 2019

codecov bot commented Jun 10, 2019 •

edited

Loading

WillAyd left a comment

WillAyd Jun 10, 2019

jbrockmendel Jun 10, 2019

WillAyd Jun 10, 2019

WillAyd Jun 10, 2019

jbrockmendel Jun 11, 2019

WillAyd Jun 12, 2019

WillAyd commented Jun 12, 2019

jreback Jun 12, 2019

jbrockmendel Jun 12, 2019

jreback Jun 21, 2019

jbrockmendel Jun 21, 2019

jreback Jun 21, 2019

jreback commented Jun 21, 2019

jbrockmendel commented Jun 22, 2019

WillAyd commented Jun 22, 2019

BLD: use unsigned instead of signed for lengths, avoid build warnings #26759

BLD: use unsigned instead of signed for lengths, avoid build warnings #26759

Conversation

jbrockmendel commented Jun 10, 2019

codecov bot commented Jun 10, 2019

Codecov Report

codecov bot commented Jun 10, 2019 • edited Loading

Codecov Report

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 21, 2019

jbrockmendel commented Jun 22, 2019

WillAyd commented Jun 22, 2019

codecov bot commented Jun 10, 2019 •

edited

Loading